properly escape quotes in passwords by calling to_ruby #1189
Conversation
| @@ -30,6 +30,6 @@ | |||
| username: <%= $username %> | |||
| <% } -%> | |||
| <% if $password { -%> | |||
| password: "<%= $password %>" | |||
| password: <%= stdlib::to_ruby($password) %> | |||
There was a problem hiding this comment.
On my phone so tricky to check, but can this be a Sensitive and does it handle that well?
There was a problem hiding this comment.
Technically, it can. No idea how to test this tho 🙈
In pulpcore, we're lucky, it's a String.
There was a problem hiding this comment.
I think you can pass Sensitive to epp and it will automatically unwrap it, so that's fine.
There was a problem hiding this comment.
lol. init.pp says db_password is a String, while the epp file allows Sensitive[String]:
puppet-foreman/manifests/init.pp
Line 229 in b1a2286
There was a problem hiding this comment.
going to drop the test (as it's failing now) and add sensitive support later
There was a problem hiding this comment.
#1190 says this does not work for Sensitive
There was a problem hiding this comment.
and this not even works properly, what the hell
There was a problem hiding this comment.
oh I'm stupid, I was running a non-patched installer 🙈
evgeni
force-pushed
the
escape-passwords
branch
from
44b811a
to
5b53748
Compare
database passwords can contain special characters, especially " and ' so we can't just print the value of the field enclosed by double quotes as that would break whenever the user uses a literal " in their password using to_ruby here and not to_yaml, as the former gives us correct escaping without the whole `---` and `\n` enclosing that to_yaml forces. using to_yaml would require to pass *the whole* config hash to it
evgeni
force-pushed
the
escape-passwords
branch
from
5b53748
to
95133be
Compare
|
It'd be a stupid password, but if your password was previously the string |
or maybe it will be quoted actually. Just looks a bit funky. |
|
It does work, partially by accident (don't look at the implementation in stdlib). Yes to_yaml would be better, but that requires more work as then we need to generate the whole hash with it. |
|
I'm thinking a new stdlib function just for escaping strings in a yaml specific way would be beneficial. Something that uses the low level Psych API. For a PoC... require 'psych'
require 'stringio'
def string_to_yaml_escaped_string(str)
out = StringIO.new
emitter = Psych::Emitter.new(out)
emitter.start_stream(Psych::Nodes::Stream::UTF8)
emitter.start_document([], [], true) # implicit flag set to true to suppress document start marker
emitter.scalar(
str,
nil,
nil,
false,
true,
Psych::Nodes::Scalar::DOUBLE_QUOTED
)
emitter.end_document(true) # implicit flag set to true to suppress document end marker
emitter.end_stream
out.string
end
str = 'ao&"$bc\ndef'
puts string_to_yaml_escaped_string(str) |
|
I had hoped there was some flag to omit the document header and an easier API to write out fragments. This doesn't deal with any datastructure, does it? |
Unfortunately, it looks like you've got to use the low level API to get this control. YAML scalars are strings and numbers, not arrays and hashes, so this would only really be useful for safely encoding individual strings. You could make the style configurable though using a different style constant from this list. https://docs.ruby-lang.org/en/master/Psych/Nodes/Scalar.html |
There was a problem hiding this comment.
I think in the short term this is good enough. If we need to wait for a new stdlib function it'll take too long before it's really available and I expect this will simply always quote strings instead of sometimes, which I actually prefer. The bare strings that sometimes turn into booleans or other data types scare me.
database passwords can contain special characters, especially " and '
so we can't just print the value of the field enclosed by double quotes
as that would break whenever the user uses a literal " in their password
using to_ruby here and not to_yaml, as the former gives us correct escaping
without the whole
---and\nenclosing that to_yaml forces.using to_yaml would require to pass the whole config hash to it